-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(http): Switch to memory cache provider #33901
base: main
Are you sure you want to change the base?
feat(http): Switch to memory cache provider #33901
Conversation
…cache-provider-switch
…-cache-provider-switch
…-cache-provider-switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will test first
@@ -13,7 +13,6 @@ async function matchingBranches( | |||
): Promise<string[]> { | |||
const { body: branches } = await githubApi.getJson( | |||
`/repos/${repo}/git/matching-refs/heads/${branchName}`, | |||
{ memCache: false }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why the memCache=false parts are no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memCache=false
is the equivalent of the absent cacheProvider
option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, no memory cache is the default now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we be confident that we haven't missed anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, we can't. What we can is analyze HTTP stats and watch for repeating GET requests (sometimes, though, it's intended).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you summarize all the cases where http memcache is still being explicitly enabled in this PR (seems to be within platform code) and we evaluate whether we're better to add platform-level caching (e.g. of PR lists) instead of relying on http-level? Maybe you can do this through inline PR comments
Changes
Make caching of GET/HEAD HTTP requests disabled by default.
When it's needed, the
memCacheProvider
is be used.Context
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: